Implemented features found in issues #6 #18
Conversation
Area Get tests implemented
Implementing Area GetNeighbours Tests
|
Hi @Janacek I got asked to take a look at this PR. Firstly thanks for picking this. I have a few small issues, the first is Spaces vs Tabs. If you could correctly format your PR in the same style as the original repository that would be great. The others I will point out in comments. |
Tests/TwoDimension/AreaTests.cs
Outdated
| Assert.Fail(); | ||
| } | ||
| Area area = new Area(); | ||
| var mockLevel = new Mock<ILevel>(); |
There was a problem hiding this comment.
Is this necessary? I don't think to perform this test you need to use the SetPosition(level, pos) on the area. This is used when we have multiple areas, and do not think it should affect any methods that are looking inside the area directly such as get neighbours.
There was a problem hiding this comment.
Indeed, this is not useful for this test, and neither for PositionGetNeighbours test.
Tests/TwoDimension/AreaTests.cs
Outdated
| } | ||
| } | ||
| Area area = new Area(); | ||
| var mockLevel = new Mock<ILevel>(); |
Tests/TwoDimension/AreaTests.cs
Outdated
| Assert.IsTrue(area.GetNeighbours(mockTile.Object).Count == 2); | ||
| mockNotANeighbourTile.Object.SetPosition(area, notANeighbourTilePosition); | ||
| area.Add(mockNotANeighbourTile.Object); | ||
| Assert.IsTrue(area.GetNeighbours(mockTile.Object).Count == 2); |
There was a problem hiding this comment.
Do you think this is an adequate test? For instance is someone is making this change and it happens that the new change actually returns two wrong tiles rather than the two tiles you are expecting.
I'd like to hear your opinions on this because Unit Tests are quite a hot potato topic in the company, it's nice to hear other peoples views on how verbose Unit Tests should be.
There was a problem hiding this comment.
This test would be irrelevant if someone were to change the test, or if the GetNeighbours method would change to return only the direct neighbours (left, top, right, bottom).
One way to fix this would be to check if the tiles we know are neighbours to the tested tile are present in the list returned by the method.
There was a problem hiding this comment.
One way to fix this would be to check if the tiles we know are neighbours to the tested tile are present in the list returned by the method.
This is exactly what I was getting at. Do you believe this is better or worse in this case?
There was a problem hiding this comment.
Since the method may change, I believe the actual test is not reliable and should be rewriten.
| throw new ArgumentException("Tile not in Area", "tile"); | ||
| } | ||
|
|
||
| List<ITile> neigbourTiles = tiles.FindAll(t => |
There was a problem hiding this comment.
I find it more readable for the delegate passed in to functions like this to be separately declared as an action, however this is quite short. What is your opinion on this?
There was a problem hiding this comment.
Since the function would need to return a boolean, an action is not possible (Maybe I'm misunderstood on this point).
I came up with this solution : (the indentation is broken here)
Func<ITile, Predicate<ITile>> ArePositionsNeighbours = lhs => (rhs => Math.Abs(((Position2D)lhs.Position).X - ((Position2D)rhs.Position).X) == 1 || Math.Abs(((Position2D)lhs.Position).Y - ((Position2D)rhs.Position).Y) == 1);
Which can be used like this :
List<ITile> neigbourTiles = tiles.FindAll(ArePositionsNeighbours(tile));
This method uses a Predicate, which is what List.FindAll needs, and allows the code to be separated. However, the code may be even less readable using this solution. On the other hand, using this, we gain re-usability, which in my opinion is really important.
There was a problem hiding this comment.
Makes sense, in this case knowing it's not going to be reused it's safe to leave it as is.
I've added both the Get and GetNeighbours methods from the class Area, with the unit tests.
The GetNeighbours method actually returns up to 8 neighbours (top-left, top, top-right, left, right, bottom-left, bottom, bottom-right)
I've assumed that since the Area position uses IPosition2D, the same goes for the Tiles added to that area.